packages/opencode/src/altimate/native/connections/data-diff.ts (1)

815-820: Cross-warehouse detection is a purely textual compare.

sourceWarehouse !== targetWarehouse treats undefined vs an explicit name as "cross-warehouse" even when the explicit name is the default (first) warehouse. The side-specific CTE injection is a strict superset of the combined prefix, so correctness isn't affected — just a minor inefficiency (slightly narrower CTE sent). Not blocking.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/opencode/src/altimate/native/connections/data-diff.ts` around lines
815 - 820, Summary: crossWarehouse currently does a raw textual compare
(sourceWarehouse !== targetWarehouse) which can misclassify undefined vs an
explicit default warehouse name as cross-warehouse. Fix: canonicalize both sides
before comparing by resolving undefined/empty values to the effective/default
warehouse name used elsewhere (use the same resolver used for
connection/effective settings) instead of comparing raw params.source_warehouse
and params.target_warehouse; update the variables referenced (sourceWarehouse,
targetWarehouse, crossWarehouse) so crossWarehouse = resolvedSourceWarehouse !==
resolvedTargetWarehouse. Ensure you call the existing resolution helper (or add
one) rather than inventing a new default string.

packages/drivers/test/sqlserver-unit.test.ts (1)

483-489: Assertion gap lets the "mixed named + unnamed" column-naming bug slip through.

This test verifies rows for { name: "alice", "": [42] } but doesn't assert result.columns. Under the current implementation (sqlserver.ts Lines 210-217), mixing a named column with multiple unnamed values (e.g. { name: "alice", "": [42, 100] }) collapses the header to ["col_0", "col_1", "col_2"] and drops name. Adding a case with ≥ 2 unnamed values and asserting columns would have caught that and will guard whichever fix lands.

♻️ Suggested additional case

     test("handles mix of named and unnamed columns", async () => {
       mockQueryResult = {
         recordset: [{ name: "alice", "": [42] }],
       }
       const result = await connector.execute("SELECT * FROM t")
       expect(result.rows).toEqual([["alice", 42]])
     })
+
+    test("preserves named-column header when mixed with multiple unnamed columns", async () => {
+      // SELECT name, COUNT(*), SUM(x) FROM t → { name: "alice", "": [42, 100] }
+      mockQueryResult = {
+        recordset: [{ name: "alice", "": [42, 100] }],
+      }
+      const result = await connector.execute("SELECT name, COUNT(*), SUM(x) FROM t")
+      expect(result.rows).toEqual([["alice", 42, 100]])
+      expect(result.columns[0]).toBe("name")
+    })

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/drivers/test/sqlserver-unit.test.ts` around lines 483 - 489, Add a
new test variant in packages/drivers/test/sqlserver-unit.test.ts that uses a
recordset like { name: "alice", "": [42, 100] } (i.e., multiple unnamed values)
and assert both result.rows and result.columns so the header preserves the named
column "name" and includes generated column names for the unnamed values (e.g.,
["name","col_1","col_2"]); this will surface the bug referenced around
sqlserver.ts (logic near lines handling column mapping) and prevent regressions
in connector.execute's column assembly.

packages/opencode/test/altimate/data-diff-dialect.test.ts (1)

51-54: Consider expanding case-insensitivity coverage.

The case-insensitivity block only exercises sqlserver and postgresql. Since mssql, fabric, and mariadb go through the same normalization path, a one-line additional assertion (e.g., MSSQL → tsql, FABRIC → fabric) would guard against a future regression where only some branches get lowercased.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/opencode/test/altimate/data-diff-dialect.test.ts` around lines 51 -
54, Add assertions to the "handles uppercase input" test to cover other inputs
that share the normalization path so case-insensitivity can't regress: update
the test block in packages/opencode/test/altimate/data-diff-dialect.test.ts that
calls warehouseTypeToDialect to also assert uppercase variants such as "MSSQL"
-> "tsql", "FABRIC" -> "fabric" and "MARIADB" -> "mariadb" (or whichever
expected dialects map in warehouseTypeToDialect) so all branches that are
lowercased are exercised.

packages/drivers/src/sqlserver.ts (1)

89-100: execSync blocks the event loop during connect().

execSync("az account get-access-token ...") with a 15 s timeout runs synchronously on the main thread inside an async function — any other pending I/O on the runtime is stalled while Azure CLI spins up (typically 0.5–2 s, occasionally much longer). Prefer the async execFile (or promisify(exec)) so the token fetch cooperates with the event loop.

♻️ Suggested change

-            try {
-              const { execSync } = await import("node:child_process")
-              const out = execSync(
-                "az account get-access-token --resource https://database.windows.net/ --query accessToken -o tsv",
-                { encoding: "utf-8", timeout: 15000, stdio: ["pipe", "pipe", "pipe"] },
-              ).trim()
-              if (out) token = out
-            } catch {
-              // az CLI not installed or not logged in
-            }
+            try {
+              const { execFile } = await import("node:child_process")
+              const { promisify } = await import("node:util")
+              const run = promisify(execFile)
+              const { stdout } = await run(
+                "az",
+                ["account", "get-access-token", "--resource", "https://database.windows.net/", "--query", "accessToken", "-o", "tsv"],
+                { encoding: "utf-8", timeout: 15000 },
+              )
+              const out = stdout.trim()
+              if (out) token = out
+            } catch {
+              // az CLI not installed or not logged in
+            }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/drivers/src/sqlserver.ts` around lines 89 - 100, The current token
fetch inside connect() uses sync child process invocation (execSync) which
blocks the event loop; replace it with an asynchronous call (e.g., use
child_process.execFile or promisified exec via util.promisify) so the Azure CLI
call runs without blocking. Locate the block that imports execSync and sets
token (the token variable and the try/catch that calls "az account
get-access-token ...") and change it to call the async API (await the
execFile/promisified exec) with the same args and timeout, handle stdout.trim()
to set token, and preserve the existing catch behavior for missing CLI or auth
failures.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/drivers/src/sqlserver.ts (2)

255-262: ⚠️ Potential issue | 🟡 Minor

Fallback to mssql.connect() still defeats per-pool isolation.

This was raised in a prior review and remains unresolved. Since packages/drivers/package.json now pins mssql@^12.0.0 where ConnectionPool is a stable named export, MssqlConnectionPool should never be falsy in practice; if it somehow is, silently using the global shared pool re-introduces the cross-database interference bug the explicit ConnectionPool path was added to fix. Throw instead.

🔒️ Proposed fix

-      if (MssqlConnectionPool) {
-        pool = new MssqlConnectionPool(mssqlConfig)
-        await pool.connect()
-      } else {
-        pool = await mssql.connect(mssqlConfig)
-      }
+      if (!MssqlConnectionPool) {
+        throw new Error(
+          "mssql.ConnectionPool is not available — the installed `mssql` package is too old. Upgrade to mssql@^12.",
+        )
+      }
+      pool = new MssqlConnectionPool(mssqlConfig)
+      await pool.connect()

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/drivers/src/sqlserver.ts` around lines 255 - 262, The fallback
branch that calls mssql.connect() reintroduces the global shared pool bug;
replace the fallback with a hard failure instead of silently using the global
pool: when MssqlConnectionPool is falsy, throw a descriptive error indicating
ConnectionPool is required (referencing MssqlConnectionPool, ConnectionPool, and
the pool initialization logic) so callers know the driver cannot operate without
the explicit ConnectionPool; do not call mssql.connect() anywhere in this
module.

---

302-312: ⚠️ Potential issue | 🟡 Minor

Mixed named + multi-unnamed columns still drop named headers.

Already flagged in an earlier review and not addressed. For SELECT name, COUNT(*), SUM(x) FROM t (recordset row { name: "alice", "": [42, 100] }), namedKeys.length is 2 but sampleFlat.length is 3, so the branch falls through to synthesizing col_0..col_2, losing the real name header. Preserve the known prefix and only synthesize names for the flattened tail.

♻️ Proposed fix

-      const rows = limitedRecordset.map(flattenRow)
-      const sampleFlat = rows.length > 0 ? rows[0] : []
-      const namedKeys = recordset.length > 0 ? Object.keys(recordset[0]) : []
-      const columns =
-        namedKeys.length === sampleFlat.length
-          ? namedKeys
-          : sampleFlat.length > 0
-            ? sampleFlat.map((_: any, i: number) => `col_${i}`)
-            : (result.recordset?.columns
-                ? Object.keys(result.recordset.columns)
-                : [])
+      const rows = limitedRecordset.map(flattenRow)
+      const sampleFlat = rows.length > 0 ? rows[0] : []
+      const sampleRow = recordset[0] ?? {}
+      const columns: string[] = []
+      if (sampleFlat.length === 0) {
+        if (result.recordset?.columns) columns.push(...Object.keys(result.recordset.columns))
+      } else {
+        let i = 0
+        for (const [k, v] of Object.entries(sampleRow)) {
+          if (k === "" && Array.isArray(v)) {
+            for (let j = 0; j < v.length; j++) columns.push(`col_${i++}`)
+          } else {
+            columns.push(k === "" ? `col_${i}` : k)
+            i++
+          }
+        }
+      }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/drivers/src/sqlserver.ts` around lines 302 - 312, The code currently
drops known named headers when some columns are flattened (variables:
limitedRecordset, flattenRow, rows, sampleFlat, namedKeys, columns), so change
the columns construction to preserve the namedKeys prefix and only synthesize
names for any extra flattened entries: if namedKeys.length === sampleFlat.length
use namedKeys; if namedKeys.length < sampleFlat.length set columns =
[...namedKeys, ...sampleFlat.slice(namedKeys.length).map((_, i) =>
`col_${namedKeys.length + i}`)]; otherwise (sampleFlat shorter) keep the current
fallback logic that uses sampleFlat or result.recordset?.columns as before.
Ensure this logic replaces the current ternary chain building columns.

packages/drivers/test/sqlserver-unit.test.ts (1)

176-181: ⚠️ Potential issue | 🟡 Minor

recordset_columns is still not a real mssql shape.

Previously flagged; the key remains incorrect. mssql exposes metadata at result.recordset.columns, not as a sibling recordset_columns, and the driver (see sqlserver.ts line 310) reads result.recordset?.columns. The test passes only because recordset is empty, so future readers will copy the wrong pattern.

💚 Proposed fix

-      mockQueryResult = { recordset: [], recordset_columns: {} }
+      const recordset: any = []
+      recordset.columns = {}
+      mockQueryResult = { recordset }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/drivers/test/sqlserver-unit.test.ts` around lines 176 - 181, The
test constructs a fake mssql result with the wrong metadata shape—using
mockQueryResult.recordset_columns instead of the real mssql shape
result.recordset.columns—so update the test ("empty result returns correctly" in
packages/drivers/test/sqlserver-unit.test.ts) to build mockQueryResult with
recordset: [] and an empty columns object under recordset.columns (i.e.,
mockQueryResult.recordset = Object.assign([], { columns: {} })) so the driver
code in sqlserver.ts (which reads result.recordset?.columns) exercises the
correct shape when calling connector.execute("SELECT * FROM t").

packages/opencode/test/altimate/data-diff-cross-dialect.test.ts (1)

114-127: Strengthen the same-warehouse joindiff positive-path assertions.

This test only checks result.success === true, which passes as long as no guard fires — it won’t catch regressions where joindiff silently degrades (e.g., falls back to hashdiff, skips execution, or produces empty SQL). Since the negative test above asserts sqlLog.length === 0, the symmetric positive assertion is cheap and much stronger.

🧪 Suggested additional assertions

     const result = await runDataDiff({
       source: "dbo.orders",
       target: "dbo.orders_v2",
       key_columns: ["id"],
       source_warehouse: "shared",
       target_warehouse: "shared",
       algorithm: "joindiff",
     })
     expect(result.success).toBe(true)
+    // Joindiff must actually dispatch SQL to the shared warehouse
+    expect(sqlLog.length).toBeGreaterThan(0)
+    expect(sqlLog.every((x) => x.warehouse === "shared")).toBe(true)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/opencode/test/altimate/data-diff-cross-dialect.test.ts` around lines
114 - 127, The test "same-warehouse joindiff is allowed" currently only checks
result.success; strengthen it by also asserting that the joindiff actually
executed SQL instead of silently degrading: after calling runDataDiff (the test
using Registry.setConfigs and runDataDiff), assert that sqlLog (the request/SQL
capture used in other tests) contains entries (e.g., sqlLog.length > 0) and that
the executed statement or logged payload indicates joindiff behavior (e.g.,
contains "JOIN" or the joindiff-specific marker), so failures where joindiff
falls back or produces no SQL are caught.

packages/drivers/src/sqlserver.ts (2)

170-186: execSync blocks the event loop inside an async path.

The Azure CLI fallback uses synchronous execSync with a 15-second timeout inside acquireAzureToken(), which will stall the entire event loop while waiting for az account get-access-token. Prefer execFile promisified (or spawn) so that other tasks (e.g. in-flight pool requests, other connectors initializing in parallel) continue to progress. Also, passing the command as a single formatted string through execSync is a minor shell-injection concern if resourceUrl is ever derived from untrusted input — execFile with an argv array avoids that entirely.

♻️ Suggested approach

-            try {
-              const { execSync } = await import("node:child_process")
-              const out = execSync(
-                `az account get-access-token --resource ${resourceUrl} --query accessToken -o tsv`,
-                { encoding: "utf-8", timeout: 15000, stdio: ["pipe", "pipe", "pipe"] },
-              ).trim()
+            try {
+              const { execFile } = await import("node:child_process")
+              const { promisify } = await import("node:util")
+              const execFileP = promisify(execFile)
+              const { stdout } = await execFileP(
+                "az",
+                ["account", "get-access-token", "--resource", resourceUrl, "--query", "accessToken", "-o", "tsv"],
+                { encoding: "utf-8", timeout: 15000 },
+              )
+              const out = stdout.trim()
               if (out) {
                 token = out
                 expiresAt = parseTokenExpiry(out)
               }
             } catch (err: any) {
               azCliStderr = String(err?.stderr ?? err?.message ?? "").slice(0, 200).trim()
             }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/drivers/src/sqlserver.ts` around lines 170 - 186, The use of
synchronous execSync inside acquireAzureToken blocks the event loop; replace it
with a non-blocking child process call (e.g., promisified execFile or spawn) to
run "az" with arguments ["account","get-access-token","--resource",
resourceUrl,"--query","accessToken","-o","tsv"], capture stdout/stderr
asynchronously, enforce the 15s timeout via timers or the child process API, and
on success set token and expiresAt (via parseTokenExpiry(out)); on failure
capture stderr/message into azCliStderr (trim to 200 chars) and preserve
existing error-handling flow so behavior of token, expiresAt, and azCliStderr
remains the same.

---

140-204: Concurrent cache-miss calls will parallel-fetch the same token.

If two connect() calls for the same (resource, clientId) start before either populates the cache, both will independently run DefaultAzureCredential.getToken() (or the az CLI) and both write to the cache. This is wasteful for AAD throttling and surprising during app startup where several diffs connect in parallel. Caching the in-flight Promise<string> (rather than the resolved token) de-duplicates concurrent acquisitions for free — and on rejection you simply evict and let the next caller retry.

const inflight = new Map<string, Promise<string>>()
// inside acquireAzureToken:
const existing = inflight.get(cacheKey)
if (existing) return existing
const p = (async () => { /* existing acquisition body */ })()
  .finally(() => inflight.delete(cacheKey))
inflight.set(cacheKey, p)
return p

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/drivers/src/sqlserver.ts` around lines 140 - 204, acquireAzureToken
currently allows concurrent cache-miss callers to run duplicate AAD/az CLI
fetches; introduce an inflight Map<string, Promise<string>> (e.g., inflight)
keyed by cacheKey and at the start of acquireAzureToken return an existing
inflight.get(cacheKey) if present, otherwise wrap the entire acquisition logic
in an async Promise, set inflight.set(cacheKey, promise) before awaiting it, and
in promise.finally() delete the inflight entry so failed attempts can be
retried; keep using tokenCache as before (check cached token first, write
resolved token into tokenCache) and on rejection ensure the inflight entry is
removed so subsequent callers will create a new fetch.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/drivers/src/sqlserver.ts (1)

147-218: Consider single-flight dedup for concurrent cold-cache token acquisitions.

If two (or many) connect() calls for the same (resourceUrl, clientId) race before the first token lands in the cache, each call runs the full DefaultAzureCredential → az path independently — exactly the "wasteful, risks throttling" scenario the cache comment flags. A small in-flight map (Map<string, Promise<string>>) next to tokenCache collapses concurrent cold requests to a single network roundtrip.

♻️ Suggested approach

 const tokenCache = new Map<string, { token: string; expiresAt: number }>()
+const inflightTokenRequests = new Map<string, Promise<string>>()

Then in acquireAzureToken:

         const acquireAzureToken = async (): Promise<string> => {
           const cached = tokenCache.get(cacheKey)
           if (cached && cached.expiresAt - Date.now() > TOKEN_REFRESH_MARGIN_MS) {
             return cached.token
           }
+          const existing = inflightTokenRequests.get(cacheKey)
+          if (existing) return existing
+          const p = (async () => {
+            // ... existing acquisition + cache-set logic ...
+            return token!
+          })().finally(() => inflightTokenRequests.delete(cacheKey))
+          inflightTokenRequests.set(cacheKey, p)
+          return p
         }

Low priority — only matters when multiple pools spin up concurrently at process start — but it's cheap insurance for the documented motivation.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/drivers/src/sqlserver.ts` around lines 147 - 218, acquireAzureToken
can run multiple concurrent cold-cache fetches; add a single-flight map (e.g.
const inFlight = new Map<string, Promise<string>>()) alongside tokenCache and
use cacheKey as the key: at start of acquireAzureToken check if
inFlight.has(cacheKey) and return the stored Promise; if not, create the Promise
that performs the existing token acquisition logic (DefaultAzureCredential path
then az CLI fallback), store it in inFlight before awaiting internals, and
ensure you remove(inFlight) in both fulfillment and rejection handlers so
failures don't permanently block retries; keep existing tokenCache.set and
TOKEN_REFRESH_MARGIN_MS logic unchanged.

packages/drivers/test/sqlserver-unit.test.ts (1)

119-131: Optional: drop the execSync mock branch now that production uses async exec.

The driver no longer calls execSync (it was replaced with promisify(exec) in packages/drivers/src/sqlserver.ts Line 183-189). The execSync stub here is dead mock-surface — keeping it risks implying execSync is still a supported fallback path in the driver.

♻️ Suggested cleanup

 mock.module("node:child_process", () => ({
   ...realChildProcess,
-  execSync: (cmd: string) => {
-    cliState.lastCmd = cmd
-    if (cliState.throwError) {
-      const e: any = new Error(cliState.throwError.message ?? "az failed")
-      e.stderr = cliState.throwError.stderr
-      throw e
-    }
-    return cliState.output
-  },
   exec: execStub,
 }))

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/drivers/test/sqlserver-unit.test.ts` around lines 119 - 131, Remove
the dead execSync branch from the test mock: in the mock.module call that
currently spreads realChildProcess and defines execSync and exec, delete the
execSync property and its closure (the cliState handling and thrown Error
creation) so only exec: execStub remains; keep the realChildProcess spread and
existing execStub usage (referencing mock.module, execStub, and cliState) to
reflect that production now uses the async promisified exec path.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes